feat(evaluation): independent retrieved-learning sampling; retire retrieval-log capture#334
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis change removes retrieval-log capture APIs and models, adds independent agent-success and retrieved-learning sampling, introduces bounded retrieved-learning retries, and expands retrieved-learning storage interfaces with exhaustive window reads. ChangesRetrieved-learning evaluation lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GenerationService
participant Sampling
participant GroupEvaluationScheduler
participant EvaluationRunner
GenerationService->>Sampling: select evaluation-family flags
Sampling-->>GenerationService: return agent-success and retrieved-learning flags
GenerationService->>GroupEvaluationScheduler: enqueue sampled evaluation
GroupEvaluationScheduler->>EvaluationRunner: run_group_evaluation(flags)
EvaluationRunner-->>GroupEvaluationScheduler: return retrieved-learning status
GroupEvaluationScheduler->>GroupEvaluationScheduler: retry failed or pending status up to 3 times
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
reflexio/server/services/agent_success_evaluation/runner.py (1)
110-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale step numbering in comments/docstring after inserting the admission gate.
The docstring's "Steps" list (lines 110-121) still enumerates the old 8-step flow and doesn't mention the new per-family admission step. Additionally, both the "already evaluated" check (Line 188) and "fetch interactions" (Line 218) are now labeled step "4", since inserting "3. Per-family admission" shifted subsequent steps without renumbering them.
✏️ Suggested renumbering
- # 4. Check if agent success is already evaluated — skipped in + # 5. Check if agent success is already evaluated — skipped in ... - # 4. Fetch interactions for all requests + # 6. Fetch interactions for all requestsAlso update the docstring "Steps" list to include the new per-family admission step.
Also applies to: 171-188, 218-218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/agent_success_evaluation/runner.py` around lines 110 - 121, Update the Steps docstring in the evaluation runner to include the per-family admission step and renumber all subsequent steps through the final operation-state update. Correct the inline step labels near the already-evaluated check and interaction fetching so each label matches the updated flow, without changing runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/services/storage/sqlite_storage/playbook/_eval_results.py`:
- Around line 417-419: The status selection in the affected retrieval method
currently treats an explicitly empty statuses override as unset. Update the
logic around the statuses parameter to use TERMINAL_RETRIEVED_STATUSES only when
statuses is None, while preserving an empty frozenset so it matches no persisted
state.
---
Nitpick comments:
In `@reflexio/server/services/agent_success_evaluation/runner.py`:
- Around line 110-121: Update the Steps docstring in the evaluation runner to
include the per-family admission step and renumber all subsequent steps through
the final operation-state update. Correct the inline step labels near the
already-evaluated check and interaction fetching so each label matches the
updated flow, without changing runtime behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ec1a1635-fb53-4431-8518-04a4bb889aff
📒 Files selected for processing (20)
reflexio/models/api_schema/domain/entities.pyreflexio/models/api_schema/domain/governance.pyreflexio/models/config_schema.pyreflexio/server/extensions.pyreflexio/server/services/agent_success_evaluation/runner.pyreflexio/server/services/agent_success_evaluation/sampling.pyreflexio/server/services/generation_service.pyreflexio/server/services/storage/retention.pyreflexio/server/services/storage/sqlite_storage/playbook/_eval_results.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/_retrieval_log.pyreflexio/server/services/storage/storage_base/playbook/_eval_results.pyreflexio/server/services/storage/storage_base/retrieved_learning_state.pyreflexio/server/services/unified_search_service.pytests/server/services/agent_success_evaluation/test_sampling.pytests/server/services/storage/test_storage_contract_retrieval_logs.pytests/server/services/storage/test_storage_contract_retrieved_learning_evals.pytests/server/services/test_generation_service_scheduling.pytests/server/services/test_unified_search_service.pytests/server/services/test_unified_search_session_dedup.py
💤 Files with no reviewable changes (7)
- reflexio/server/services/storage/storage_base/_retrieval_log.py
- tests/server/services/storage/test_storage_contract_retrieval_logs.py
- tests/server/services/test_unified_search_session_dedup.py
- reflexio/server/services/storage/storage_base/init.py
- reflexio/models/api_schema/domain/entities.py
- tests/server/services/test_unified_search_service.py
- reflexio/server/services/unified_search_service.py
…rieval-log capture Reconciled onto OSS main (per-interaction RLE, #348): keeps main's evaluation_version freshness check + the new statuses-param terminal-state match. Independent retrieved_learning_sampling_rate + bounded retry sweep + retrieval-log retirement (AuditEntityType literal kept for read-compat). SETTLED/statuses machinery retained here; dropped in a follow-up per the reconciliation design.
The retrieved-learning-evals contract helper is now _result(storage, kind, learning_id), but the window-read tests added by #334 still called it with 2 args, raising TypeError at collection. Update every 2-arg call site to pass the backend handle.
…NAL-only The offline tuner consumes TERMINAL retrieved-learning state only. main already retries degraded -> complete, so a degraded session self-heals on the next scheduled run and is picked up once it settles. This reverts the #334 SETTLED surface to main's param-less form: - Remove SETTLED_RETRIEVED_STATUSES from retrieved_learning_state. - Remove the statuses= param from get_matching_retrieved_learning_terminal_state on both the abstract base and SQLite backend; match TERMINAL_RETRIEVED_STATUSES directly (preserving the evaluation_version + fingerprint fence). - Drop the two SETTLED-specific storage contract tests; refresh dangling SETTLED_RETRIEVED_STATUSES comments in generation_service and its scheduling test (degraded is re-judged fresh on the next run, not read directly).
f7f3758 to
a34c3e5
Compare
Paired with the enterprise PR that rewires the offline playbook tuner onto a single per-playbook impact signal. Merge this first, then the enterprise repo pins it.
Why
The tuner now reads the per-playbook
impactverdict inretrieved_learning_evaluationand nothing else. Two consequences land in OSS:Interaction.retrieved_learningsnow carries.What's here
Independent per-family sampling
New
AgentSuccessConfig.retrieved_learning_sampling_rate, defaultNone= inheritsampling_rate— an org that never opts in keeps exactly its previous behavior and cost.New
agent_success_evaluation/sampling.py: the publish gate samples both families once, schedules when either admits the session, and passes two booleans to the runner. One computation site, so the families cannot diverge. A session sampled only for retrieved-learning never pays the session-success judge — which is the point: it lets an operator buy coverage without also raising the agent-success bill.Direct callers (regen jobs, on-demand grade) run both families as before. Sampling is a scheduling decision, not a runner one.
degradedis settled, not brokenfailed/pendingcommitted nothing and had no retry trigger, so such a session was invisible forever. The publish callback now re-arms it on the existing scheduler, bounded to 3 attempts.degradedis deliberately NOT retried. It is an applied, fingerprint-fenced commit: the rows are persisted and only the failed chunks carry NULL impact. Retrying it re-executes every relevance+impact chunk and delete/re-inserts already-committed rows — and a deterministically degrading chunk (over-length learning, content-filter refusal) degrades again on every attempt, so a bounded sweep burned 4× the judge bill and bought nothing.Instead: new
SETTLED_RETRIEVED_STATUSES(=TERMINAL | {degraded}), exposed via an optionalstatusesarg on the terminal-state fence, so consumers read those rows directly. Contract-tested that widening the set does not weaken the freshness check — a degraded session whose transcript changed is still rejected.Exhaustive windowed read
get_retrieved_learning_evaluation_results_in_window()on the base + SQLite. The existing paged reader hard-caps atlimit=100with no cursor; a consumer that treats the window as the complete set must not inherit that silent truncation. Contract-tested at 0 / 100 / 101 / 250 rows.Retrieval-log retirement
Deletes
PlaybookRetrievalLog/PlaybookRetrievalLogItem,RetrievalLogMixin, and theconfigure_retrieval_capture_hooksearch hook.Deliberately KEPT:
AuditEntityType"playbook_retrieval_log"literal — historicalaudit_eventsrows still carry it, and removing it makes Pydantic reject them on read;Verification
main.Noneexplicitly, so it asserted its own argument — which is exactly how a0.1default drifted in unnoticed. Verified the new test goes red if the drift is reintroduced.Summary by CodeRabbit
retrieved_learning_sampling_rateconfig (inherits existing sampling when unset).playbook_retrieval_logsduring the retirement period.